drivers/enc28j60: fix ISR routine and bth #19461

peteut · 2023-04-10T15:48:04Z

Contribution description

When the /drivers/enc28j60 driver was in use, many resets have been observed. A closer look revealed there is an SPI access in ISR context, which can lead to deadly embraces:

RIOT/drivers/enc28j60/enc28j60.c

Line 253 in 6f0ac0c

cmd_bfc((enc28j60_t *)arg, REG_EIE, -1, EIE_INTIE);

To fix this issue, the SPI access has been moved to thread context.

Another potential issue is the Receive Packet Pending Interrupt Flag (PKTIF), which is not reliable. Refer to
ENC28J60 Silicon Errata and Data Sheet Clarification, section 6 Module: Interrupts.

To address this issue, the proposed workaround from the errata has been implemented.

Testing procedure

./RIOT/dist/tools/benchmark_udp run on the host and USEMODULE += benchmark_udp on the target has been used for testing. No more freezes have been observed. The network stack in use was lwip.

A patch has been applied to test on IP4:

diff --git a/sys/test_utils/benchmark_udp/benchmark_udp.c b/sys/test_utils/benchmark_udp/benchmark_udp.c
index dd41972508..d8043c4556 100644
--- a/sys/test_utils/benchmark_udp/benchmark_udp.c
+++ b/sys/test_utils/benchmark_udp/benchmark_udp.c
@@ -144,11 +144,10 @@ static void *_send_thread(void *ctx)
 
 int benchmark_udp_start(const char *server, uint16_t port)
 {
-    netif_t *netif;
-    sock_udp_ep_t local = { .family = AF_INET6,
+    sock_udp_ep_t local = { .family = AF_INET,
                             .netif = SOCK_ADDR_ANY_NETIF,
                             .port = port };
-    sock_udp_ep_t remote = { .family = AF_INET6,
+    sock_udp_ep_t remote = { .family = AF_INET,
                              .port = port };
 
     /* stop threads first */
@@ -159,15 +158,11 @@ int benchmark_udp_start(const char *server, uint16_t port)
         return 1;
     }
 
-    if (netutils_get_ipv6((ipv6_addr_t *)&remote.addr.ipv6, &netif, server) < 0) {
+    if (netutils_get_ipv4((ipv4_addr_t *)&remote.addr.ipv4, server) < 0) {
         puts("can't resolve remote address");
         return 1;
     }
-    if (netif) {
-        remote.netif = netif_get_id(netif);
-    } else {
-        remote.netif = SOCK_ADDR_ANY_NETIF;
-    }
+    remote.netif = SOCK_ADDR_ANY_NETIF;
 
     running = true;
     thread_create(listen_thread_stack, sizeof(listen_thread_stack),

$ make -C $RIOTBASE/dist/tools/benchmark_udp/ run
...
host                	bandwidth	num TX	num RX		num RT		RTT	pkg size
::ffff:192.168.1.118	 477 b/s	25885	25885 (100%)	25879 (99%)	1918 µs	48

$ make info-stm32 
CPU: stm32wb55vg
	Line: STM32WB55xx
	Pin count:	
	ROM size:	824K (843776 Bytes)
	RAM size:	256KiB

$ make info-packages 
lwip
tinyusb

Issues/PRs references

Fixes #17593.

riot-ci · 2023-04-10T23:00:17Z

Murdock results

✔️ PASSED

226b8cf drivers/enc28j60: fix PKTIF issue.

Success	Failures	Total	Runtime
6882	0	6882	09m:36s

Artifacts

Documentation preview

benpicco

Looks good to me and you provided testing results.
Just a whitespace nitpick:

drivers/enc28j60/enc28j60.c

The PKTIF does not reliably report the status of pending packags. Apply the proposed workaround [1]. [1] https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ProductDocuments/Errata/80349c.pdf

benpicco · 2023-04-12T13:21:43Z

bors merge

19411: cpu/gd32v: add riotboot support r=benpicco a=gschorcht ### Contribution description This PR provides `riotboot` support for GD32V. ### Testing procedure Use any GD32V board with a JTAG adapter and flash the bootloader: ```python PROGRAMMER=openocd BOARD=sipeed-longan-nano make -C bootloaders/riotboot flash ``` Flash slot 0 and set `RIOT_VERSION` to 1 ```python USEMODULE=stdio_uart FEATURES_REQUIRED=riotboot RIOT_VERSION=1 \ PROGRAMMER=openocd BOARD=sipeed-longan-nano make -C tests/shell riotboot/flash-slot0 ... ### Flashing Target ### Binfile detected, adding ROM base address: 0x08000000 Flashing with IMAGE_OFFSET: 0x08001000 ``` ```python > main(): This is RIOT! (Version: 1) test_shell. ``` Flash slot 1 and set `RIOT_VERSION` to 2 ```python USEMODULE=stdio_uart FEATURES_REQUIRED=riotboot RIOT_VERSION=2 \ PROGRAMMER=openocd BOARD=sipeed-longan-nano make -C tests/shell riotboot/flash-slot1 ... ### Flashing Target ### Binfile detected, adding ROM base address: 0x08000000 Flashing with IMAGE_OFFSET: 0x08010800 ``` ```python > main(): This is RIOT! (Version: 2) test_shell. ``` ### Issues/PRs references 19432: boards/esp32: deduplication in common ESP32x board definitions r=benpicco a=gschorcht ### Contribution description The PR reduced code duplication in `boards/common/esp32*`. The PR moves the header files from `boards/common/esp32s3/include` that can be used for all types of ESP32x SoCs to a new common ESP32x board definition which is then included by all common ESP32x board definitions. ### Testing procedure Green CI. ### Issues/PRs references 19461: drivers/enc28j60: fix ISR routine and bth r=benpicco a=peteut Co-authored-by: Gunar Schorcht <gunar@schorcht.net> Co-authored-by: Alain Péteut <alain.peteut@yahoo.com>

bors · 2023-04-12T13:49:08Z

Build failed (retrying...):

Murdock

bors · 2023-04-12T16:08:31Z

Build succeeded:

drivers/enc28j60: do not block in ISR ctx.

200c49d

peteut requested a review from haukepetersen as a code owner April 10, 2023 15:48

github-actions bot added the Area: drivers Area: Device drivers label Apr 10, 2023

peteut marked this pull request as draft April 10, 2023 15:48

peteut force-pushed the enc28j60-fixes branch from 4132a08 to 4bf4c4a Compare April 10, 2023 15:52

peteut changed the title ~~Enc28j60 fixes~~ /drivers/enc28j60: fix ISR routine and bth Apr 10, 2023

peteut marked this pull request as ready for review April 10, 2023 16:35

benpicco requested a review from maribu April 10, 2023 22:46

benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 10, 2023

benpicco changed the title ~~/drivers/enc28j60: fix ISR routine and bth~~ drivers/enc28j60: fix ISR routine and bth Apr 10, 2023

benpicco requested a review from jia200x April 10, 2023 22:51

benpicco approved these changes Apr 11, 2023

View reviewed changes

drivers/enc28j60/enc28j60.c Outdated Show resolved Hide resolved

drivers/enc28j60: fix PKTIF issue.

226b8cf

The PKTIF does not reliably report the status of pending packags. Apply the proposed workaround [1]. [1] https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ProductDocuments/Errata/80349c.pdf

peteut force-pushed the enc28j60-fixes branch from 4bf4c4a to 226b8cf Compare April 11, 2023 10:26

bors bot merged commit 99b13cb into RIOT-OS:master Apr 12, 2023

peteut deleted the enc28j60-fixes branch April 12, 2023 17:31

benpicco added this to the Release 2023.07 milestone Aug 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers/enc28j60: fix ISR routine and bth #19461

drivers/enc28j60: fix ISR routine and bth #19461

peteut commented Apr 10, 2023 •

edited

Loading

riot-ci commented Apr 10, 2023 •

edited

Loading

benpicco left a comment

benpicco commented Apr 12, 2023

bors bot commented Apr 12, 2023

bors bot commented Apr 12, 2023

drivers/enc28j60: fix ISR routine and bth #19461

drivers/enc28j60: fix ISR routine and bth #19461

Conversation

peteut commented Apr 10, 2023 • edited Loading

Contribution description

Testing procedure

Issues/PRs references

riot-ci commented Apr 10, 2023 • edited Loading

Murdock results

Artifacts

benpicco left a comment

Choose a reason for hiding this comment

benpicco commented Apr 12, 2023

bors bot commented Apr 12, 2023

bors bot commented Apr 12, 2023

peteut commented Apr 10, 2023 •

edited

Loading

riot-ci commented Apr 10, 2023 •

edited

Loading